test(parallel): deterministic rendezvous + drop node:fs from timing-sensitive fixtures#34992
test(parallel): deterministic rendezvous + drop node:fs from timing-sensitive fixtures#34992robobun wants to merge 5 commits into
Conversation
…JEST_WORKER_ID test Under debug+ASAN (post #34782), worker startup takes longer than 200ms, so worker 0 finishes its file and work-steals worker 1's file before worker 1 sends .ready. Both files then print WID=1 and the test sees [1,1,3]. Each fixture now appends its PID to a shared gate file and polls until three lines appear, guaranteeing all three worker slots have a file in flight before any can finish. The assertion is unchanged (sorted WIDs == [1,2,3], JEST_WORKER_ID == BUN_TEST_WORKER_ID), plus a new check that the three files ran on three distinct PIDs. The K<=1 serial-fallback half uses a non-gated fixture since there is only one process.
WalkthroughChangesParallel CLI tests now coordinate workers deterministically and collect worker, partitioning, and work-stealing data from stderr instead of temporary log files. Parallel test determinism
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:59 PM PT - Jul 21st, 2026
❌ @robobun, your commit 10dce75 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34992That installs a local version of the PR into your bun-34992 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only de-flake replacing a fixed Bun.sleep(200) with a gate-file rendezvous.
What was reviewed:
- Rendezvous logic: each worker appends before reading, so
readFileSync(GATE)never hits ENOENT;trimEnd().split("\n").lengthcounts lines correctly; concurrentappendFileSyncof <20 bytes is atomic under O_APPEND (same pattern already used forPIDS/LOGelsewhere in this file). - K≤1 serial-fallback half correctly switched to a non-gated fixture (would otherwise hang on undefined
GATE/ never reach 3 lines); itstoContain("WID=1 1")assertion still matches the newpid=-suffixed output. - Assertions are strictly stronger (added distinct-PID check), nothing weakened;
String(dir) + "/gate"matches the mixed-separator convention used throughout the file.
Extended reasoning...
Overview
Single-file, test-only change to test/cli/test/parallel.test.ts, touching one test case (--parallel: each worker has a unique JEST_WORKER_ID and BUN_TEST_WORKER_ID). The fixed 200ms sleep is replaced with a gate-file rendezvous so no worker can finish (and work-steal) until all three worker slots have a file in flight. The second half of the test (K≤1 serial fallback) gets its own trivial non-gated fixture. A new assertion checks that three distinct PIDs ran the three files.
Security risks
None. Test-only; no runtime code, no new dependencies, no network, all I/O confined to a tempDir.
Level of scrutiny
Low. This is a targeted CI de-flake with a well-argued root cause (ASAN worker boot > 200ms after #34782 → worker 0 steals worker 1's file before it boots). The fix is exactly what REVIEW.md prescribes — await the observable condition instead of a fixed sleep — and reuses the append-PID-to-shared-file pattern already established in three other tests in this same file. No production code paths change.
Other factors
- Checked the poll loop can't spuriously pass: each worker appends its own line before reading, so the file always exists and the count is monotone; small O_APPEND writes don't interleave.
- Checked the serial-fallback half wasn't accidentally left pointing at the gated fixture (it would hang) — it now inlines a plain fixture that shares only the
printsnippet. - Checked no existing assertions were weakened: the sorted
["1","2","3"]andJEST_WORKER_ID == BUN_TEST_WORKER_IDchecks remain, and the distinct-PID assertion is strictly additive. - No CODEOWNERS entry covers this path; no prior human review comments to address.
…xtures Under debug+ASAN --isolate, evaluating the node:fs module costs ~500-800ms per file. The scale-up / partitioning / work-stealing fixtures imported it solely to appendFileSync a pid marker, which made every 'fast' file slow enough to exceed the fast-case scale-up threshold and pushed the 16-file partition run past the default 5s test timeout. - JEST_WORKER_ID rendezvous: Bun.write + Bun.Glob instead of fs - lazily-scales / partitions / work-stealing: console.error the marker and parse it from the coordinator-captured stderr (per-pid order is preserved by flush_captured at test_done) No assertions weakened; no src/ changes.
robobun
changed the title
There was a problem hiding this comment.
LGTM — well-reasoned test de-flake replacing a fixed sleep with a deterministic rendezvous.
What was reviewed:
- Gate rendezvous:
Bun.Glob("*")defaultsdot: falseso.keepis excluded and the poll waits for exactly 3 PID files; no deadlock since each worker writes before polling and can't finish/steal until all three have. - K≤1 serial-fallback half correctly switched to a non-gated fixture (the gated one would hang waiting for 3 files that never appear).
appendFileSync→console.error("ROW=...")swap: per-PID dispatch order is preserved (coordinator flushes at test_done, workers run files sequentially), which is all thebyPidgrouping needs; the work-stealing test is order-independent.
Extended reasoning...
Overview
Test-only de-flake of test/cli/test/parallel.test.ts. Two commits: (1) replace Bun.sleep(200) in the JEST_WORKER_ID test with a gate-file rendezvous so no worker can finish and work-steal before all three slots have a file in flight, and give the K≤1 serial-fallback half its own non-gated fixture; (2) drop import "fs" / appendFileSync from three timing-sensitive fixtures (lazily scales, partitions by directory, work-stealing balances) in favor of console.error markers parsed from stderr, since the node:fs re-eval under debug+ASAN+isolate adds ~800ms per file and blows the fast-case threshold.
Security risks
None. Test-only change in a spawned-subprocess harness; no production code, no untrusted input handling.
Level of scrutiny
Low-to-moderate. This is a de-flake of an existing test with a thorough root-cause analysis in the PR description (work-stealing from a not-yet-ready worker slot when ASAN boot exceeds the 200ms sleep). The fix is exactly what the repo's own review guidance prescribes: await the actual observable condition instead of a fixed sleep. I verified the one non-obvious edge — Bun.Glob("*") defaults dot: false (src/runtime/api/glob.rs:108), so the .keep placeholder in gate/ is not counted and the < 3 poll genuinely waits for all three worker PIDs.
Other factors
The assertions are strictly stronger than before (adds a 3-distinct-PIDs check on top of the sorted [1,2,3] WID check). The ROW=/PID= stderr-parsing approach relies on the same per-test output buffering that the neighboring never interleaves console output test already verifies, and the new comment explicitly states the ordering invariant it depends on. All spawns still drain stdout/stderr/exited concurrently. No existing assertions were weakened or removed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/test/parallel.test.ts`:
- Line 581: Add concise comments for both intentional long delays in the
parallel tests: at test/cli/test/parallel.test.ts lines 581-581, explain how the
150ms sleep preserves the partitioning observation; at lines 621-621, explain
why slow a/ work is required to exercise stealing. Ensure each comment states
the durable test invariant.
- Around line 5-8: Condense the gate-rendezvous comment in
test/cli/test/parallel.test.ts lines 5-8 to no more than three lines, retaining
only the durable rationale that it prevents early work-stealing during slow
worker startup. Also condense the node:fs performance rationale at lines 521-524
to no more than three lines, preserving only its non-obvious durable
justification.
- Line 13: The worker-gate polling loop in the parallel test must have a
deadline instead of waiting indefinitely. Update the loop around
Array.fromAsync(new Bun.Glob("*").scan(process.env.GATE)) to stop after a
bounded interval and throw a diagnostic identifying the missing three-worker
rendezvous; preserve the existing polling behavior while the deadline has not
expired.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ebc84e4a-291e-4dc6-bfca-e9d7bad5ae0b
📒 Files selected for processing (1)
test/cli/test/parallel.test.ts
There was a problem hiding this comment.
LGTM — well-reasoned test de-flake, no src/** changes. The inline nit about partitions by directory losing its zero-rows guard is worth the one-line addition but doesn't block.
Extended reasoning...
Overview
Test-only change to test/cli/test/parallel.test.ts fixing four timing-sensitive tests that went flaky on the debian-13 x64-asan lane after #34782 shifted debug+ASAN startup costs. The JEST_WORKER_ID test replaces a fixed Bun.sleep(200) with a gate-directory rendezvous (each worker writes $GATE/<pid> and polls until three entries exist), so no worker can finish and work-steal before all three have a file in flight. Three other tests (lazily scales, partitions by directory, work-stealing balances) drop import {appendFileSync} from "fs" from their fixtures — which under debug+ASAN+isolate costs ~800ms per file — in favor of console.error markers parsed from coordinator-forwarded stderr.
Security risks
None. Test fixtures only; no network, no untrusted input parsing, no src/** changes.
Level of scrutiny
Low-to-moderate. This is CI de-flaking of existing tests with a clearly diagnosed root cause (work-stealing from not-yet-booted worker slots is intended behavior; the fixed sleep just stopped covering ASAN startup latency). The rendezvous pattern is the repo-recommended "await the condition, don't sleep" approach. The K≤1 serial-fallback half correctly switches to a non-gated fixture (the gated one would deadlock waiting for three pids). The stderr-marker approach is consistent with the existing never interleaves test in the same file, which already depends on worker console.error lines appearing at column 0 in coordinator stderr — so ^ROW= / ^PID= anchoring rests on tested behavior.
Other factors
- Assertions are strengthened, not weakened: the JEST_WORKER_ID test now also asserts three distinct pids. The one exception is the inline nit —
partitions by directorylost its implicit zero-rows failure (old code threw ENOENT/SyntaxError on an empty side-file; new regex yields[]→0 === 0). That's a hypothetical future-regression guard, not a current defect, and the siblingwork-stealingtest'sexpect(aRows.length).toBe(8)would still catch a stderr-forwarding regression. Bun.Glob("*")skipping the.keepdotfile is standard glob semantics (nodot: true), so the count-to-3 is correct.- CodeRabbit's three inline threads are all marked resolved; the "bound the polling loop" suggestion is unnecessary since the inner runner's default per-test timeout already bounds the rendezvous and would surface as
exitCode !== 0. - PR description shows
bun bd testat 27/0 pass and includes the debug/ASAN evidence log.
There was a problem hiding this comment.
LGTM — test-only de-flake, and the rows.length === 16 guard from the earlier note is in.
Checked: the gate rendezvous can't deadlock (worker 0 blocks in the poll so it can't finish-and-steal; Glob("*") default dot:false skips .keep); the K≤1 fallback correctly uses a non-gated fixture; the stderr ^PID=/^ROW= parsing relies on the same line-for-line forwarding the never interleaves test already asserts; assertions are net-strengthened (distinct-pid check, 16-row guard).
Extended reasoning...
Overview
Single-file change to test/cli/test/parallel.test.ts fixing debug+ASAN flakiness in four tests. The JEST_WORKER_ID test replaces a fixed Bun.sleep(200) with a gate-directory rendezvous so no worker can finish (and work-steal) before all three have a file in flight. Three timing-sensitive tests (lazily scales, partitions by directory, work-stealing balances) drop import "fs" from fixtures — which costs ~800ms/file under debug+ASAN+isolate — and instead emit PID=/ROW= markers on stderr, parsed via regex from the coordinator's captured output. No src/** changes.
Security risks
None. Test-only; fixtures run in ephemeral tempDirs under the debug binary; no network, no external inputs, no auth/crypto surface.
Level of scrutiny
Low-to-medium. This is a test de-flake with a precise root-cause narrative (work-stealing from not-yet-spawned slots when ASAN worker startup exceeds the old 200ms sleep), verified 27/27 pass under bun bd test, and no runtime behavior touched. The main things worth checking were whether the rendezvous can wedge and whether the stderr-parsing approach silently weakens any assertion — both examined below.
Other factors
- Rendezvous correctness: with
BUN_TEST_PARALLEL_SCALE_MS=0and 3 files/3 workers, worker 0 writes its gate file then spins in the poll loop, so it cannot complete and steal; workers 1/2 spawn, write their gate files, and all three release together.Bun.Glob("*").scan()defaults todot:false(src/runtime/api/glob.rs:108), sogate/.keepis not miscounted toward the threshold. The K≤1 serial-fallback half was correctly switched to a non-gated fixture (a gated one would wait forever for 3 pids). If the coordinator ever fails to spawn a third worker the loop is bounded by the outer test timeout — a loud failure, not a silent pass. - Assertion strength: my earlier nit about
partitions by directorygoing vacuous on zero ROW matches was addressed in 10dce75 withexpect(rows.length).toBe(16). TheJEST_WORKER_IDtest gained a distinct-pid assertion.lazily scalesandwork-stealingretain their non-vacuous guards (pids.sizechecks,aRows.length === 8). Net: stronger than before. - stderr channel: the
^ROW=/^PID=anchoring depends on the coordinator forwarding worker stderr at column 0, which the siblingnever interleavestest in the same file already asserts. The added comment documents that ROW lines are flushed attest_doneso per-PID ordering matches dispatch order (all thebyPid[pid][0]assertion needs). - Prior review feedback: CodeRabbit's comment-length nit was addressed in 968ef84; the pre-existing
sleep(150)/sleep(250)in the partition/steal fixtures were not introduced by this PR and are already explained by the surrounding block comments.
|
Diff is green: The only hard-red in build 77176 is CI infra on Ready to merge. |
1 participant
What
test/cli/test/parallel.test.ts→--parallel: each worker has a unique JEST_WORKER_ID and BUN_TEST_WORKER_IDwent red on the debian-13 x64-asan lane (build 77013, plus retried sightings in 76807 and 76821):Under a local
bun bd(debug+ASAN) build the whole file additionally failslazily scales workers/partitions by directory/work-stealing balances, which pass on the CI asan lane but sit right at (or past) the 5s default there.Cause
Not an id-assignment bug. Each worker slot still gets its own
JEST_WORKER_ID(sloti→envps[i]→ idi+1;Worker::start()indexes byself.idx). The duplicate1is work-stealing: the fixture doesawait Bun.sleep(200)so worker 0 stays busy while workers 1/2 boot, but under debug+ASAN worker startup now exceeds 200ms, so worker 0 finishesa.test.js,steal_back_halftakes the single file out of worker 1's range before worker 1 sends.ready, and bothaandbrun in the same process (assign_workexplicitly allows stealing from not-yet-spawned slots; this is intended).Reproduces 5/5 on a local
bun bdbuild:For the other three, the fixtures
import {appendFileSync} from "fs"purely to record a pid marker. Under debug+ASAN--isolate, evaluating the fullnode:fsmodule costs ~500-800ms per file (measured:bun-debug -e 'import {appendFileSync} from "fs"'→ ~1.18s vs ~0.40s baseline). That overhead alone exceeds the 250ms fast-case scale-up threshold inlazily scales, pushes the 16-filepartitions by directoryrun to ~4.9s against the default 5s timeout, and erases the slow/fast distinctionwork-stealing balancesdepends on.Both tipped after #34782 (debian-13 build host, ThinLTO, WebKit bump, new rust toolchain): the race has existed since #29413, the margin moved.
Fix
Bun.write($GATE/<pid>, "")then pollsBun.Glob("*").scan($GATE)until three entries appear, so no worker can finish (and steal) until all three slots have a file in flight.Bun.Glob("*")skips the.keepdotfile that pre-creates the directory. Assertions unchanged (sorted[1,2,3],JEST_WORKER_ID == BUN_TEST_WORKER_ID), plus a check that three distinct pids ran. The K≤1 serial-fallback half now uses a non-gated fixture.import "fs"from the fixtures and write the pid /{pid,file}marker withconsole.error(...)instead; parse it out of the coordinator-captured stderr with/^PID=/m//^ROW=/m. The coordinator flushes each worker's captured bytes attest_done, so within a pid the ROW lines appear in dispatch order (which is all thebyPid[pid][0]assertions need). NoappendFileSyncside-file, no per-filenode:fsevaluation.No assertions weakened; no
src/**changes.Verification
partitions by directorydrops from ~4.97s to ~1.98s underbun bd;work-stealing balancesfrom ~4.35s to ~1.78s.[stamp-90s] gate passed · iteration 3 · 1 files touched
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 1 rejected · iteration 3
evidence per changed file